fix(assets): pass through images Sharp cannot decode instead of crashing#16451
fix(assets): pass through images Sharp cannot decode instead of crashing#16451maximslo wants to merge 6 commits intowithastro:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 0d3f832 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
@maximslo
Thank you for your PR!
It seems faild some tests. I think it related new imolementation of this PR.
Will you check this? And I recomend you to create test for this issue. Thank you!
|
@fkatsuhiro Added! You can run it with I believe all earlier failing tests were MiniflareCoreError [ERR_RUNTIME_FAILURE], the Cloudflare Workers runtime (infrastructure/environment issue with the CI runner, not anything touched by my change) |
There was a problem hiding this comment.
@maximslo san
Thank you for your updating!
I think it looks good to me. But I forgot to tell you to create changeset file. Sorry.
You can create changeset file via command pnpm changeset. And select your updated package and write summery of your change. Could you update your PR?
Reference: https://contribute.docs.astro.build/docs-for-code-changes/changesets/#tips-and-examples
Sorry for bothering.
fkatsuhiro
left a comment
There was a problem hiding this comment.
Thanks for your updating!
Looks good to me! Thanks!
|
What does have a passthrough achieve? if you have animated avif that isn't supported, you just get an unoptimized version, right? But in this case, can't you just not use the Image component for this? Not 100% against this, just my initial thought is that it might be better to through, because by using Image you are asking for optimizations, and not getting them might be more confusing than throughing (in which case you can just avoid using the Image component). |
|
@matthewp Yes, users could just use Without this fix: The build crashes (exit code 1) so they have no choice. Must either delete the image, convert it, or move it to public/. To address your concern with the fix, I'd suggest logging a warning for both SVG and AVIF: With this fix + warning: The build succeeds and users see a clear warning message that tells them:
Some might prefer to convert the AVIF and use I think warning + passing through is better UX than crashing, because it unblocks the user, and a warning would clear up any confusion/inform them of the issue. |
|
Pinging @Princesseuh for her opinion on this. |
|
I think with a warning it's okay. Ultimately we wouldn't hide the problem like this, but since there's no real solution on the user side (because in SSR, remote images, uploads, etc you might not be able to know what you're processing), a warning and not crashing seems like a better UX, I agree. |
This is a fix for #16267.
Changes
metadata()call in a try/catch. When Sharp cannot decode an image (e.g. animated AVIF sequences), the original buffer is returned unmodified instead of crashing the build. The file is still hashed for cache-busting; only transformation is skipped._astro/filename.avifin the build output. The<img>tag in the HTML still points to that file; the browser receives the original animated AVIF directly.public/) is that Astro still hashes the filename for cache-busting. The content is identical to the source file.Testing
Run the new test directly:
pnpm run test:match "animated avif"OR manually:
packages/astro/test/fixtures/core-image-ssg/src/assets/<Image>in a page.pnpm buildThe build should complete and the file should appear in
_astro/ unmodified. Previously this would crash with CouldNotTransformImage.Note: future support for AVIF would require...
The groundwork is already partly there.
pages: -1is the Sharp flag that loads all frames of an animated image (already set) BUT needed:Sharp/libvips version support. The crash itself (
metadata()throwing) means the current Sharp (0.34.5) can't decode animated AVIF metadata. This is a libvips limitation, newer builds of libvips with libaom support can handle it. Depends on Sharp upstream.Format detection. AVIF uses the HEIF container, so Sharp reports animated AVIF as
format: 'heif', notavif. The animated GIF → WebP path insharp.tswould need an equivalent:result.avif()withpages: -1already loaded should output animated AVIF, but only once Sharp can decode it in the first place.So this pass-through fix is fine short-term. Full support is blocked on Sharp's libvips gaining animated AVIF decode support.
Docs
/cc @withastro/maintainers-docs for feedback!